Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add test-dds-adapter-depth test #13605

Open
wants to merge 6 commits into
base: development
Choose a base branch
from

Conversation

maloel
Copy link
Collaborator

@maloel maloel commented Dec 12, 2024

Adds basic streaming test for rs-dds-adapter.

Moved the librs library from dds-specific code to rspy, so other tests can use it.

Tracked on [LRS-1217]

@maloel maloel added ci CI-related: does not change library behavior dds labels Dec 12, 2024
@maloel maloel requested a review from OhadMeir December 12, 2024 17:04
@OhadMeir
Copy link
Contributor

Moved the librs library from dds-specific code to rspy, so other tests can use it.

I am not sure librs is the appropriate name for this in rspy scope. Was named librs because it was a helper to test librealsense DDS capabilities (also testing realdds in same tests folder). Maybe incorporate it into devices.py?
What do you think?

Copy link
Contributor

@OhadMeir OhadMeir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thought about it, we have a live/tools unit test folder. I think the adapter test should be there. Maybe under an internal dds folder

@maloel
Copy link
Collaborator Author

maloel commented Dec 15, 2024

Moved the librs library from dds-specific code to rspy, so other tests can use it.

I am not sure librs is the appropriate name for this in rspy scope. Was named librs because it was a helper to test librealsense DDS capabilities (also testing realdds in same tests folder). Maybe incorporate it into devices.py? What do you think?

It's librealsense, with everything that it has, plus some additions for unit-testing (but not for dds). Has nothing to do with dds.

@maloel
Copy link
Collaborator Author

maloel commented Dec 15, 2024

Thought about it, we have a live/tools unit test folder. I think the adapter test should be there. Maybe under an internal dds folder

I don't agree: everything for dds should be under dds. The 'live' folder is not always right, and using it to group things does not always make sense. That's why we have --live and --not-live as switches for run-unit-tests. It was a good grouping at the time, but it's no longer needed.

If you insist, I'll change.

@OhadMeir
Copy link
Contributor

Thought about it, we have a live/tools unit test folder. I think the adapter test should be there. Maybe under an internal dds folder

I don't agree: everything for dds should be under dds. The 'live' folder is not always right, and using it to group things does not always make sense. That's why we have --live and --not-live as switches for run-unit-tests. It was a good grouping at the time, but it's no longer needed.

If you insist, I'll change.

This test should fail if no USB camera is attached. How is it marked to run only with live cameras now?

@maloel
Copy link
Collaborator Author

maloel commented Dec 15, 2024

Thought about it, we have a live/tools unit test folder. I think the adapter test should be there. Maybe under an internal dds folder

I don't agree: everything for dds should be under dds. The 'live' folder is not always right, and using it to group things does not always make sense. That's why we have --live and --not-live as switches for run-unit-tests. It was a good grouping at the time, but it's no longer needed.
If you insist, I'll change.

This test should fail if no USB camera is attached. How is it marked to run only with live cameras now?

--live does not check the tag or folder - it checks whether it has a test:device directive in it.

@maloel
Copy link
Collaborator Author

maloel commented Dec 16, 2024

--live does not check the tag or folder - it checks whether it has a test:device directive in it.

Not sure you saw this... I got no email...

@OhadMeir
Copy link
Contributor

OK if --live is looking for device directive, but I don't see that your test is running. It is skipped both for GHA and LibCI.

@maloel
Copy link
Collaborator Author

maloel commented Dec 18, 2024

I don't see that your test is running

Good catch.
The reason is that this test is live and DDS at the same time, and we have no setup that runs both together...
We have to make LibCI compile DDS (possibly only on nightly?) @Nir-Az any preference?

@Nir-Az
Copy link
Collaborator

Nir-Az commented Dec 18, 2024

I don't see that your test is running

Good catch. The reason is that this test is live and DDS at the same time, and we have no setup that runs both together... We have to make LibCI compile DDS (possibly only on nightly?) @Nir-Az any preference?

Did a test run, build time increase from 36 [m] to 45[m]
For nightly only it's OK

1 thing I am not sure from the code is if dds on will run only non live test or both live and not live?

@maloel
Copy link
Collaborator Author

maloel commented Dec 18, 2024

1 thing I am not sure from the code is if dds on will run only non live test or both live and not live?

I will make it so that, on nightly only, LibCI will compile DDS. This will be a configuration in Jenkins.
This will also cause the nightly to run all tests -- not just live -- i.e., including DDS -- and this test would then run.

@maloel
Copy link
Collaborator Author

maloel commented Jan 3, 2025

I had a lot of trouble - the adapter test failed in LibCI under Jenkins, and Dima could not help. It turns out that the std::cin.ignore( ... ) code doesn't work under jenkins: cin reports it's at EOF and the adapter quits right away.

To get around this, I implemented proper Control-C handling in Windows/Linux and added it to rsutils.

You can see the run here:
https://rsjenkins.iil.intel.com/job/LRS_libci_pipeline/9859/

std::unique_lock< std::mutex > lock( _m );
_is_set = true;
}
_cv.notify_all();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn' t notify_all also be under the lock?

In case thread A calls set and immediately after thread B calls clear_and_set we will want thread B to be blocked. Now we can have a case where set lock is released, thread B waits, then thread A continues and notify the cv.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn' t notify_all also be under the lock?

No, it doesn't have to be.

If you want I'll move it in but it clearly says, and demonstrates in all the examples, that it does not need to be in the lock.

In case thread A calls set and immediately after thread B calls clear_and_wait

If a notify is warranted, then a notify should happen. If you want nobody to clear it until all the notify work is done, then you need to make sure the notifications are part of the mutex, but there's no way to do that without actually calling a notification callback from within the lock.

Usually the more likely scenario is that, on notification, a thread would then say 'OK let me wait for the next one' so would do a clear_and_wait. There's no way to guarantee a notification-per-set on a simple event mechanism. If you want that, use a signal object.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know it doesn't have to be locked.
Keeping it under the lock can help with a certain race scenario, were set and clear_and_wait are called (almost) simultaneously. It won't prevent the race itself, and I agree it is not the common use case.
Your call if to keep like this or change.

@OhadMeir
Copy link
Contributor

OhadMeir commented Jan 5, 2025

I will make it so that, on nightly only, LibCI will compile DDS. This will be a configuration in Jenkins. This will also cause the nightly to run all tests -- not just live -- i.e., including DDS -- and this test would then run.

Is this handled?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci CI-related: does not change library behavior dds
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants